Add support for parsing orbital moments from OUTCAR #3463
Conversation
|
@oashour Is this still WIP? If not, can you update the title? |
oashour
changed the title
|
@janosh done. The [WIP] tag was mainly because I'm a first-time contributor, and thought there might be some comments, but I have nothing to add to this unless someone spots a problem or possible improvement. |
janosh
force-pushed
the
master
branch
2 times, most recently
from
3c23114
to
36e289c
Compare
janosh
force-pushed
the
master
branch
4 times, most recently
from
d725325
to
dca98be
Compare
|
sorry this slipped. i forgot this is waiting on maintainer review.
if you can do that in a backwards compatible manner, that'd be much apprecciated! |
|
I can definitely do that, I only wrote it that way to avoid refactoring too much before talking to you folks. I'll work on this soon.
|
janosh
added
enhancement
WalkthroughThe Changes
Recent Review DetailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
i just resolved the merge conflicts. @oashour please ping us when this is ready to go |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Out of diff range and nitpick comments (2)
tests/io/vasp/test_outputs.py (2)
Line range hint
14-14: Ensure theTODOfor adding tests is addressed or tracked in an issue.Would you like me to help by generating the unit testing code or opening a GitHub issue to track this task?
Line range hint
25-27: Optimize the modulo operation.- while x >= y: - x -= y - return x + return x % y
pymatgen/io/vasp/outputs.py
Outdated
| orbital_moment (tuple): Orbital moments on each ion as a tuple of dict, e.g., | ||
| ({"d": 0.109, "p": -0.001, "tot": 0.108}, ... ) |
There was a problem hiding this comment.
Ensure the orbital_moment attribute is correctly documented and initialized.
- orbital_moment (tuple): Orbital moments on each ion as a tuple of dict, e.g.,
+ orbital_moment: list = field(default_factory=list) # Orbital moments on each ion as a list of dict, e.g.,Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| orbital_moment (tuple): Orbital moments on each ion as a tuple of dict, e.g., | |
| ({"d": 0.109, "p": -0.001, "tot": 0.108}, ... ) | |
| orbital_moment: list = field(default_factory=list) # Orbital moments on each ion as a list of dict, e.g., | |
| ({"d": 0.109, "p": -0.001, "tot": 0.108}, ... ) |
pymatgen/io/vasp/outputs.py
Outdated
| orbmom_x = [] | ||
| orbmom_y = [] | ||
| orbmom_z = [] |
There was a problem hiding this comment.
Initialize orbmom_x, orbmom_y, and orbmom_z to handle orbital moments correctly.
- orbmom_x = []
- orbmom_y = []
- orbmom_z = []
+ orbmom_x: list = field(default_factory=list)
+ orbmom_y: list = field(default_factory=list)
+ orbmom_z: list = field(default_factory=list)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| orbmom_x = [] | |
| orbmom_y = [] | |
| orbmom_z = [] | |
| orbmom_x: list = field(default_factory=list) | |
| orbmom_y: list = field(default_factory=list) | |
| orbmom_z: list = field(default_factory=list) |
pymatgen/io/vasp/outputs.py
Outdated
| read_orbmom_x = False # For SOC calculations with LORBMOM=.TRUE. | ||
| read_orbmom_y = False | ||
| read_orbmom_z = False |
There was a problem hiding this comment.
Ensure read_orbmom_x, read_orbmom_y, and read_orbmom_z are initialized properly to handle reading orbital moments.
- read_orbmom_x = False # For SOC calculations with LORBMOM=.TRUE.
- read_orbmom_y = False
- read_orbmom_z = False
+ read_orbmom_x: bool = False # For SOC calculations with LORBMOM=.TRUE.
+ read_orbmom_y: bool = False
+ read_orbmom_z: bool = FalseCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| read_orbmom_x = False # For SOC calculations with LORBMOM=.TRUE. | |
| read_orbmom_y = False | |
| read_orbmom_z = False | |
| read_orbmom_x: bool = False # For SOC calculations with LORBMOM=.TRUE. | |
| read_orbmom_y: bool = False | |
| read_orbmom_z: bool = False |
pymatgen/io/vasp/outputs.py
Outdated
| read_orbmom_x = False | ||
| read_orbmom_y = False | ||
| read_orbmom_z = False |
There was a problem hiding this comment.
Ensure flags for reading orbital moments are reset correctly.
- read_orbmom_x = False
- read_orbmom_y = False
- read_orbmom_z = False
+ read_orbmom_x: bool = False
+ read_orbmom_y: bool = False
+ read_orbmom_z: bool = FalseCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| read_orbmom_x = False | |
| read_orbmom_y = False | |
| read_orbmom_z = False | |
| read_orbmom_x: bool = False | |
| read_orbmom_y: bool = False | |
| read_orbmom_z: bool = False |
janosh
force-pushed
the
master
branch
2 times, most recently
from
e3fbc67
to
41e6d99
Compare
|
@janosh This should be good to go. I'm sorry it took so long--it's been hard to find the time to write the code I actually want to write. The same approach could be used to clean up the code for the magnetization, but that should be done in another PR. |
Summary
Major changes:
OUTCAR.NiO_SOC.gz.Todos
I opened this as a WIP since I have not contributed to
pymatgenbefore, but it's technically complete. As far as I know, VASP does not write orbital moments anywhere besidesOUTCAR, but if it does, I'd be happy to implement a parser if there's interest.To minimize changes, I kept the code style consistent with the magnetization parsing in the
Outcarclass. However, it's a bit ugly due to the three extra boolean variables. I'm happy to rewrite that entire part to make it more compact.The final thing to note is that the spin contribution to the magnetization is always with respect to
SAXIS(there's a#TODOcomment for that in the code), whereas orbital magnetization is always in Cartesian coordinates. Thus, thesaxisproperty of the resultantMagmomobjects for the orbital moments (the default[0 0 1]) is correct, but it's not generally correct for the spin contribution to the magnetic moments.Checklist
ruff.Type annotations included. Check with(none needed)mypy.If applicable, new classes/functions/modules have(not applicable)duecredit@due.dcitedecorators to reference relevant papers by DOI (example)